app: improve IME composition handling#171
Conversation
SnippetCmd can grow to include surrounding text while an IME composition is active. If the selection has not moved and the old snippet still matches the new one, don't cancel the platform composition. Signed-off-by: qiannian <qianniancn@gmail.com>
Send the editor's visible composition bounds with SelectionCmd and use them as the CFS_EXCLUDE rectangle for ImmSetCandidateWindow. That puts the candidate list below the text being composed, instead of just under the caret. Handle RESULTSTR and COMPSTR as separate IME updates. Cursor-only updates move the IME cursor without replacing text in the editor. Fixes: https://todo.sr.ht/~eliasnaur/gio/697 Signed-off-by: qiannian <qianniancn@gmail.com>
whereswaldon
left a comment
whereswaldon
left a comment
There was a problem hiding this comment.
Thank you very much for working on this. I have a couple requests, but they aren't very complex:
- The implementation of the windows IME event handling feels like it could be made easier to comprehend by a refactor focusing on returning early from each category of message. I may have misunderstood the constraints on the implementation though, so feel free to correct me if my requests there aren't possible/practical.
- The way that the editor is now updating the IME state seems too expensive to do every frame; I'd like to more precisely do that work only when we must.
Overall, I think this is looking really promising. Thanks again.
| end := start + utf8.RuneCountInString(result) | ||
| w.w.SetComposingRegion(key.Range{Start: -1, End: -1}) | ||
| w.w.SetEditorSelection(key.Range{Start: end, End: end}) | ||
| state = w.w.EditorState() |
There was a problem hiding this comment.
Given that we commit the composition here and clear the composing region, do we actually need to fall through and execute the rest of this switch case, or can we early-return here?
The only interaction that I see is that maybe there are messages that set GCS_DELTASTART or GCS_CURSORPOS at the same time as GCS_RESULTSTR, but I'm not sure that can happen in practice (what would move the composing region when the composing region no longer exists?).
Early-returning here would help make the various phases of processing this message easier to understand, but we don't need to do it if there actually is a case where we get GCS_RESULTSTR and those other positioning messages together.
| } | ||
| } | ||
| w.w.SetEditorSelection(key.Range{Start: pos, End: pos}) | ||
| w.updateIMEWindows(imc) |
There was a problem hiding this comment.
We'd need to defer this if we did the early-return strategy I mentioned above.
| e.scrollCaret = false | ||
| e.text.ScrollToCaret() | ||
| } | ||
| e.updateIMEState(gtx) |
There was a problem hiding this comment.
I worry about the overhead of doing this every frame. I know that we don't actually issue a key.SelectionCmd unless the selection has changed, but traversing the text regions to resolve the composition bounds every frame seems expensive. Could we instead only do this if we know the selection, contents, scroll position, or layout constraints have changed?
| rel := windows.ImmGetCompositionValue(imc, windows.GCS_CURSORPOS) | ||
| pos = state.RunesIndex(state.UTF16Index(rng.Start) + rel) | ||
| } | ||
| w.w.SetEditorSelection(key.Range{Start: pos, End: pos}) |
There was a problem hiding this comment.
Similarly, here we handle both GCS_COMPSTR and any associated GCS_DELTASTART/GCS_CURSORPOS. Might be an opportunity to early-return?
2 participants
Place the Windows IME candidate window under the active composition text.
Also avoid cancelling IME composition when a snippet update only expands
around matching text.
Fixes: https://todo.sr.ht/~eliasnaur/gio/697